-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*) core: make ap_escape_quotes() work correctly on strings #298
Conversation
icing
commented
Mar 17, 2022
with more than MAX_INT/2 characters, counting quotes double. Credit to <generalbugs@zippenhop.com> for finding this.
Added in trunk as r1899609. |
@@ -2615,7 +2615,7 @@ AP_DECLARE(void) ap_content_type_tolower(char *str) | |||
*/ | |||
AP_DECLARE(char *) ap_escape_quotes(apr_pool_t *p, const char *instring) | |||
{ | |||
int newlen = 0; | |||
apr_ssize_t extra = 0; | |||
const char *inchr = instring; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Why apr_ssize_t and not apr_size_t?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra
is used in a ptrdiff_t
expression below, which is signed. apr_ssize_t
is ptrdiff_t
on all but the most ancients of platforms.
But is it correct to use? The question is, does it deal correctly with strings that are longer than ptrdiff_t can hold (lets call this PTRDIFF_MAX)?
- an
instring
that is longer than PTRDIFF_MAX: this would mean trouble as(inchr - instring)
in the palloc expression would overflow. Which is undefined behaviour in C. C defines that you can substract pointers into
the same array to get a number. So, maybe we can ignore that one. - an
instring
that is almost as long as PTRDIFF_MAX with enough quote characters that the resulting length is larger than PTRDIFF_MAX. This is currently not checked.
I think we need a check that the expression (inchr - instring) + extra + 1
did not overflow, e.g. became < 0, to be totally safe. And for this, we need to calculdate in apr_ssize_t
, I guess.
What do you think?
if (!extra) { | ||
return apr_pstrdup(p, instring); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe apr_pmemdup(), as we can easily compute the length, as done below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion.